Codecov Report

❌ Patch coverage is 85.54217% with 12 lines in your changes missing coverage. Please review.

📢 Thoughts on this report? Let us know!

@tadasant please review.

[EDIT: see comment below...]

I think where possible, I'd want to push this into the JSON schema spec as much as possible - this means:

• it's well documented to users of the spec
• we have fewer places we're defining the spec
• it's automatically validated by Huma (I think?)

Of course there will be exceptions to this where we really can't do it with JSON schema. But a lot of this I think could be replicated with e.g. length or regex requirements on the JSON schema etc.

Hmm actually just read up on #97, and that maybe we only want this to be on the official registry API spec (rather than the generic registry API spec). Based on @.tadasant's comment:

First step is to enumerate and add those constraints to the registry-schema.json. We don't want them in the core schema.json - the latter is fine to be more permissive.

I guess in that case the approach you've gone for here is fine - I'll leave it up to @tadasant to decide / review :)

@domdomegg - isn't it preferred to still have the validation in the schema even if it's just for registry-server.json?

@rdimitrov possibly true? I guess then it'd be harder to take the official-registry-api-schema.json and turn it into the registry-api-schema.json. But maybe this is fine and/or we shouldn't maintain them this way.

@domdomegg / @tadasant can we conclude on the discussion whether we want to add the validation in the spec and let huma handle it or we should go ahead with the code based validation? It's fine if we need more time to think it through.

Sorry I'm well behind on reviews and discussions and trying to focus where I'm blocking @domdomegg - trust his call on this one if he wants to jump ahead and get this merged (otherwise I will get to this eventually, just will be slow)

This is correct:

maybe we only want this to be on the official registry API spec

The intent is for this formatting/validation to only apply to the official registry. Most importantly at publication-time (we don't want people publishing data that is noncomformant to these expectations)

I haven't yet formed an opinion on how that cascades to the implementation here

No worries @tadasant, take your time. @domdomegg you can also review if you want to.

Okay I've had a think about this, and my conclusion is that I think it doesn't matter too much either way 😅

I have a weak preference towards doing it as schema annotations in huma, rather than custom code validations. I think this documents it better for users of the API, and usually is less verbose/puts all the schema info in one place.

I'd be happy to accept this PR, or one doing it with schema annotations, once the code is cleaned up a little (e.g. linting and tests pass). @Avish34 are you good to take up implementing it one of these ways, then pinging me once you have CI passing?

Thanks @domdomegg , Yes I'm picking up the implementation. Sure!!

@domdomegg CI is passing now. Can you start reviewing please?

I will address the comments, thanks for reviewing @domdomegg. Btw is this https://github.com/modelcontextprotocol/registry/blob/main/docs/server-json/schema.json are you talking about in the comments right?

@Avish34 yeah for the comments like 'I think this could be made a requirement on the main server.json schema' I think we'd want to add them to the server.json schema and as annotations on the Huma API probably here following these docs.

@domdomegg Got it. You mean adding minLength and maxLength to the openapi schema of server.json for the relevant fields and adding the same annotation to model.go to allow huma to validate. Is the understanding right?

Yes that's correct, and similar for other validations that should be universal to server.json (i.e. not specific to the official MCP registry)

@domdomegg I have added the validation in the schema for server.json and annotations in model.go, I tried enabling huma validation but it's failing some UTs. I think enabling I can take up in next PR if not a problem, we can merge this till then.

Thanks a lot @domdomegg !!